Skip to content

Conversation

masseyke
Copy link
Member

This is the second of three PRs that will result in the addition of component template substitutions support to the simulate ingest API. See the draft PR at #112762 for the full idea.
This PR updates TransportAbstractBulkAction and TransportSimulateBulkAction to use the substitute component templates that were added to BulkRequest in #112957.
The next PR will finish the rest layer, add the full documentation, and add rest-level testing.

@masseyke masseyke added >non-issue :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v8.16.0 v9.0.0 labels Sep 17, 2024
@masseyke masseyke marked this pull request as ready for review September 17, 2024 21:35
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Sep 17, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I left a few really minor comments

Comment on lines +208 to +209
assert (bulkRequest instanceof SimulateBulkRequest) == false
: "TransportBulkAction should never be called with a SimulateBulkRequest";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also add an assert that there are no template substitutions here? I.e., that getComponentTemplateSubstitutions() returns an empty map?

}

Builder(Metadata metadata) {
public Builder(Metadata metadata) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want this to remain protected, favoring .builder(otherMeta) instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good point! I had missed that that method existed already.

final DocWriteRequest<?> originalRequest,
final IndexRequest indexRequest,
final Metadata metadata,
Map<String, ComponentTemplate> templateSubstitutions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call this var componentTemplateSubstitutions since that's the name we've used elsewhere? (and presumably we'll have to add one for index template substitutions eventually)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes definitely. I missed that one when I renamed all the rest.

@masseyke masseyke merged commit ec50aaa into elastic:main Sep 19, 2024
15 checks passed
@masseyke masseyke deleted the adding-transport-code-for-simulate-ingest-component-templates branch September 19, 2024 22:42
masseyke added a commit to masseyke/elasticsearch that referenced this pull request Sep 20, 2024
elasticsearchmachine pushed a commit that referenced this pull request Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >non-issue Team:Data Management Meta label for data/management team v8.16.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants